-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Add System.Numerics.Matrices Namespace #1846
Conversation
Hi @whoisj, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution! TTYL, DNFBOT; |
added new MatrixTemplate.tt for generating matrix classes added 1 - 8 column by 1 - 8 row matrices classes
cd7eeb0
to
8ccadd8
Compare
@whoisj, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR. |
Just curious, what use cases do you see for such types, besides spatial/geometry,
|
@cdrnet good questions. Honestly, I don't think there's a need for larger than 4x4 matrices in corefx. I only included more to see if there was a desire. There's not much cost to providing them. Matrices are a fundamental mathematical structure. They're useful in any kind of reporting and data manipulation. Not having them isn't helping and they're "cheap" once provided. I've developed and re-developed matrix structures in several separate projects and always wished they were provided by the core library. In many cases developers are disallowed from using third-party libraries (with coreclr and corefx being exceptions). Assuming availability in a third-party library is sufficient for all, is an invalid assumption. Yes a full featured math library is a wonderful idea, but as you point out it is out of scope for corefx. Supporting basic, complex math types seems like something corefx could, and should provide. Types like vectors and matrices are simple enough and generally useful enough, in my opinion, to be provided as part of corefx. |
I'm a little disappointed that there hasn't been a warmer reception of the idea of adding matrices to the corefx. 😞 Looks like tests are failing, but I'm not sure how to look at the results and parse meaning from them. Can anyone assist me in understanding what I'm looking at when I look at Jenkins? 🙏 |
@whoisj on build details page open Console Output (in left side menu) and select there build configuration you are interesting in and than again Console Output for detailed log of building and testing. Build details: http://dotnet-ci.cloudapp.net/job/dotnet_corefx_prtest/1491/
|
@Maxwe11 thanks, took some digging and it seems |
Jenkins isn't really loading for me very well right now, so I can't try to check why the tests aren't working. @mmitche , could you help with that? Few notes on the design of the library
|
@mellinoe I'm restarting it now. There was an update to the pull request builder plugin that appears to have broken the status update bits of the PR testing. |
@mellinoe I'm agnostic about the number of matrices. Since I did a TT, I figured I could produce whatever sizes were useful. Can you be more specific about the operators you're unsure of? Matrices can multiply (or dot) so long as they are sized as the right side has as many rows as the left size does columns. If I've mucked up the logic, it's likely due to the translation from actual code to the TT. 😞 As for the overlap with the other namespace - I honestly didn't look at the other namespace closely. I saw that it was called System.Numerics.Vectors and assumed it contained vectors (and not matricies). If this work is duplicate, then I it is unneeded. |
http://dotnet-ci.cloudapp.net/job/dotnet_corefx_mac_release_prtest/815/console From the console logs: d:\j\workspace\dotnet_corefx_mac_release_prtest\src\System.Numerics.Matrices\src\System.Numerics.Matrices.csproj : error MSB4057: The target "BuildAndTest" does not exist in the project. |
@mmitche yes, I noted that above. 😄 Is there a good reference for how to setup the BuildAndTest target? Thanks! |
@whoisj You might want to check out some of the other projects that are in corefx. For instance, take a look at System.IO.Compression. The way the system is set up, it is not a stock C# project dropped into the directory structure. It appears the primary thing you're missing is an import of the dir.props and dir.targets at the beginning and end of the project file. In addition, I'd take a look at the other bits of setup the compression project is doing around the dependency resolution and references. |
@mmitche thanks! With that pointer, I should be good to go. 😃 |
@whoisj, thanks. This would be a rather large API addition, and for it to be considered for corefx it would need to go through the API review process: https://github.com/dotnet/corefx/blob/master/Documentation/api-review-process.md Rather than a PR (which wouldn't be merged, or at least wouldn't be for quite some time), seems like it'd be better either as an issue on which a discussion could happen and API goals/shapes/etc could be evolved, or if there was real interest potentially as a library over on https://github.com/dotnet/corefxlab. |
Thanks @stephentoub . I can leave the PR for the point of leaving the code behind. I've been completely swamped with other issues and unable to update the PR recently anyways. 😐 |
Corefxlab would be a good place for this. |
@KrzysztofCwalina interesting. There's more and more stuff I'm finding up here. Thanks! 🙇 |
Surely this should be fixed by education of those making such questionable decision rather than reimplementing all .Net opensource as part of CoreFx. |
This pull request is primarily to take the temperature of the maintainers and community with regards to the following:
The important file to look at is the MatrixTemplate.tt - all other code is generated from that. I believe I got my matrix logic correct, thought I have not spent a ton of time validating it yet. I fully plan to do so once I get some community feedback on my approach.
There are no tests written yet. I fully plan to write the appropriate tests once my approach is validated.
added new MatrixTemplate.tt for generating matrix classes
added 1 - 8 column by 1 - 8 row matrices classes